Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Prepare for static deep-copy in 3.8 rebase #16469

Merged
merged 4 commits into from
Sep 25, 2017
Merged

Prepare for static deep-copy in 3.8 rebase #16469

merged 4 commits into from
Sep 25, 2017

Conversation

sttts
Copy link
Contributor

@sttts sttts commented Sep 21, 2017

This has no effect yet in 3.7 based master, but will make the 3.8 rebase much easier
with the deepcopy tags in place and test types moved into non-test packages to
be reachable by deepcopy-gen.

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 21, 2017
@sttts sttts assigned ironcladlou and deads2k and unassigned fabianofranz Sep 21, 2017
@deads2k
Copy link
Contributor

deads2k commented Sep 21, 2017

/approve

For intent. I think it will be a big help for the rebase and inert (yet correct) for our code at the moment.

@openshift-merge-robot openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 21, 2017
@sttts
Copy link
Contributor Author

sttts commented Sep 21, 2017

The second commit is a pure move of the types plus the new tags.

@deads2k
Copy link
Contributor

deads2k commented Sep 21, 2017

Seems like we should have at least some customization somewhere. Squashed in an existing commit? The clusterquota copier comes to mind.

@sttts
Copy link
Contributor Author

sttts commented Sep 21, 2017

Seems like we should have at least some customization somewhere. Squashed in an existing commit? The clusterquota copier comes to mind.
Merge state

Custom .DeepCopy funcs on types will still be supported. Note that this PR does not backport the new deepcopy-gen which actually uses the tags. I.e. existing zz_generator_deepcopy.go files should not be changed here. I only added some more packages to the list in deepcopygen.go because we will need that in the 3.8 rebase as well. Technically, we could drop that change here.

@sttts sttts force-pushed the sttts-static-deepcopy-prep branch 2 times, most recently from a69e134 to d606e96 Compare September 21, 2017 13:53
@sttts
Copy link
Contributor Author

sttts commented Sep 21, 2017

/retest

1 similar comment
@deads2k
Copy link
Contributor

deads2k commented Sep 22, 2017

/retest

@deads2k
Copy link
Contributor

deads2k commented Sep 22, 2017

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 22, 2017
@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, sttts

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 23, 2017
@sttts sttts added the lgtm Indicates that a PR is ready to be merged. label Sep 23, 2017
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

3 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 25, 2017
@sttts sttts added the lgtm Indicates that a PR is ready to be merged. label Sep 25, 2017
@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue

@openshift-merge-robot openshift-merge-robot merged commit b3475a3 into openshift:master Sep 25, 2017
@jim-minter jim-minter mentioned this pull request Sep 25, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@ironcladlou ironcladlou

@soltysh soltysh

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. needs-api-review size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@sttts @deads2k @openshift-merge-robot @openshift-bot @fabianofranz @ironcladlou @soltysh @openshift-ci-robot